Fix windows `Socket::connect_timeout` overflow #112464

eval-exec · 2023-06-09T13:47:12Z

This PR want to close #112405

add unit test

rustbot · 2023-06-09T13:47:21Z

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

eval-exec · 2023-06-09T14:19:43Z

@rustbot review

library/std/src/sys/windows/net.rs

ChrisDenton · 2023-06-10T15:46:13Z

This looks good to me.

add unit test

This is a bit more tricky but I think the easiest way would be to copy/paste connect_error and then change it to use connect_timeout(&next_test_ip4(), Duration::MAX) instead of connect.

ChrisDenton · 2023-06-12T09:34:55Z

library/std/src/net/tcp/tests.rs

@@ -46,6 +46,22 @@ fn connect_error() {
    }
 }

+#[test]
+fn connect_timeout_error() {
+    match TcpStream::connect_timeout(&"0.0.0.0:1".parse().unwrap(), Duration::MAX) {


Have you confirmed this test fails without the changes in net.rs?

@eval-exec for an alternative idea of what a test could do:

bind some free port on localhost using TcpListener::bind, then

connect to that new port with Duration::MAX timeout.

Here's a draft in Rust:

use std::net::TcpListener; let listener = TcpListener::bind("127.0.0.1:0").unwrap(); // :0 picks some free port let port = listener.local_addr().unwrap().port(); // obtain the port it picked [..] [..] format!("127.0.0.1:{port}").as_str() [..] // connect to that port

I have a real example of something very close at /~https://github.com/hartwork/rust-for-it/blob/4e51982c4465c3190d92a59747c5d2738baf9e07/src/network.rs#L160-L177 .

What do you think?

Thank you, I'm testing it on a win10 virtual machine.

@eval-exec I checked the latest version: 0.0.0.0:1 will only work if you start a TCP listener on port 1. And port 1 only works if it's unoccupied and you have privileges because ports up to 1024 are reserved. In a test context you'll probably want a dynamic free port as in my example above.

I'm confused, I try to connect to an unreachable address 8.8.8.8:9999, but it didn't get timeout.

@eval-exec updated twice more (in case you only saw the version sent via notification e-mail)

It should be format!("0.0.0.0:{port}" or format!("127.0.0.1:{port}" for connect_timeout?

@eval-exec I would argue for 127.0.0.1 for clarity. Both work in practice.

I think I should bind and accept in another thread:

#[test] fn connect_timeout_ok_bind() { let socket_addr = next_test_ip4(); thread::spawn({ let listener = t!(TcpListener::bind(&socket_addr)); move || { let mut stream = t!(listener.accept()).0; let mut buf = [0]; t!(stream.read(&mut buf)); } }); assert!(TcpStream::connect_timeout(&socket_addr, Duration::MAX).is_ok()); }

I think I should bind and accept in another thread:

@eval-exec that's not necessary, see /~https://github.com/hartwork/rust-for-it/blob/4e51982c4465c3190d92a59747c5d2738baf9e07/src/network.rs#L160-L177 .

thomcc · 2023-06-17T06:32:28Z

Chris seems like he's reviewing this.

r? @ChrisDenton

hartwork · 2023-06-17T22:13:43Z

The failure mode this PR resolves is Duration::MAX being interpreted as a negative integer (I.e. less than zero time).

@ChrisDenton yes.

This causes an immediate return with a TimeOut error.

Cannot confirm, it caused infinite blocking for me as documented at #112405.

With the bug fixed Duration::MAX will be correctly interpreted and a TimeOut error will not be returned. Some other error will be but that is to be expected.

Cannot confirm, no error should be returned but success since this is about available targets.

ChrisDenton · 2023-06-17T22:17:02Z

The failure condition in your issue was "Instead, this happened: the call never succeeds, not even for google.com:80 on Windows."

That is not infinite blocking.

hartwork · 2023-06-17T23:48:39Z

The failure condition in your issue was "Instead, this happened: the call never succeeds, not even for google.com:80 on Windows."

That is not infinite blocking.

@ChrisDenton maybe I should have put it more clearly: by "never succeeds" I meant that it should have returned success quickly for google.com:80 but instead it blocked forever and hence never finishes to return with due success. Given your spot-on early analysis at #112405 (comment) I do not understand how we have different understanding now. I'd be happy to learn what I a missing now. Are there two bugs here instead of a single one?

hartwork · 2023-06-17T23:53:57Z

@ChrisDenton PS: The CI at /~https://github.com/hartwork/rust-for-it/actions/runs/5204380736/jobs/9388510541 was cancelled by GitHub after near-exact 6 hours runtime due to that call while google.com:80 was available.

eval-exec · 2023-06-18T00:01:46Z

@ChrisDenton PS: The CI at /~https://github.com/hartwork/rust-for-it/actions/runs/5204380736/jobs/9388510541 was cancelled by GitHub after near-exact 6 hours runtime due to that call while google.com:80 was available.

What's the related code of this canceled CI action?

hartwork · 2023-06-18T00:13:24Z

@ChrisDenton PS: The CI at /~https://github.com/hartwork/rust-for-it/actions/runs/5204380736/jobs/9388510541 was cancelled by GitHub after near-exact 6 hours runtime due to that call while google.com:80 was available.

What's the related code of this canceled CI action?

@eval-exec that would effectively be /~https://github.com/hartwork/rust-for-it/tree/4c816e51eb8134cef32571400f5583e3f75cec54 or:

git clone /~https://github.com/hartwork/rust-for-it
cd rust-for-it
git checkout 4c816e51eb8134cef32571400f5583e3f75cec54

eval-exec · 2023-06-18T01:08:45Z

connect to a reachable address:

use std::net::{TcpStream, ToSocketAddrs};
use std::time::Duration;

fn main() {
    let address_result = "1.1.1.1:80".to_socket_addrs();
    let address = address_result.unwrap().next().unwrap();
    TcpStream::connect_timeout(&address, Duration::MAX).expect("connect_timeout");
    println!("main fn returned");
}

On Linux platform, rustc 1.70.0, this program returned imediately, and no error.
On Windows platform, rustc 1.70.0, this program panic at an error imediately: thread 'main' panicked at 'connect_timeout: Error { kind: TimedOut, message: "connection timed out" }', src\main.rs:7:57
On Windows platform, with this PR's fix, and I run this program in a unit test, this program returned imediately, and no error

connect to an unreachable address:

use std::net::{TcpStream, ToSocketAddrs};
use std::time::Duration;

fn main() {
    let address_result = "1.1.1.1:9999".to_socket_addrs();
    let address = address_result.unwrap().next().unwrap();
    TcpStream::connect_timeout(&address, Duration::MAX).expect("connect_timeout");
    println!("main fn returned");
}

On linux platform, rustc 1.70.0, this program running 2minutes and 8second, and finaly got thread 'main' panicked at 'connect_timeout: Os { code: 110, kind: TimedOut, message: "Connection timed out" }', src/main.rs:7:57
On Windows platform, rustc 1.70.0, this program panic at an error imediately: thread 'main' panicked at 'connect_timeout: Error { kind: TimedOut, message: "connection timed out" }', src\main.rs:7:57
On Windows platform, with this PR's fix, and I run this program in a unit test, this program paniced after 21.04 seconds:

thread 'net::tcp::tests::connect_timeout_example' panicked at 'connect_timeout: Os { code: 10060, kind: TimedOut, message: "A connection attempt failed because the connected p
arty did not properly respond after a period of time, or established connection failed because connected host has failed to respond." }', library\std\src\net\tcp\tests.rs:15:88

eval-exec · 2023-06-18T01:16:22Z

So, I think I can add a unit test: connect to an unreachable address like 1.1.1.1:9999. assert connect_timeout(1.1.1.1:9999, Duration::MAX) will get a TimedOut error, and assert the action should cost at least 20 seconds.

#[test]
fn connect_timeout_to_unreachable_address() {
    let now = Instant::now();
    let result =
        TcpStream::connect_timeout(&format!("1.1.1.1:9999").parse().unwrap(), Duration::MAX);
    
    assert!(matches!(result, ErrorKind::TimedOut));
    
    assert!(now.elapsed() > Duration::from_secs(20));
}

What do you think? @hartwork @ChrisDenton

ChrisDenton · 2023-06-18T07:19:05Z

@hartwork the reason your CI ran indefinitely is because you have an infinite loop. if running only the code in the issue, it errors immediately.

Signed-off-by: Eval EXEC <execvy@gmail.com>

eval-exec · 2023-06-18T08:02:23Z

library/std/src/net/tcp/tests.rs

@@ -46,6 +46,37 @@ fn connect_error() {
    }
 }

+#[test]
+fn connect_timeout_to_unreachable_address() {


I tested, this unit test will cost at 2 minutes and 8 seconds on Linux platform, and cost 20 seconds on Windows.

I think we only need one test here. We only need to focus on testing the problematic code path in one way. 2 minutes and 8 secs is a long time for a single unit test so I'd prefer if we just used connect_timeout_error test and removed the other two.

Ok, I think 2 minutes and 8 seconds is a long time too.

hartwork · 2023-06-18T17:33:24Z

@hartwork the reason your CI ran indefinitely is because you have an infinite loop. if running only the code in the issue, it errors immediately.

@ChrisDenton thanks for clearing this up, you're 100% right, I was able to confirm your results in GitHub Actions now. Sorry 🙏

library/std/src/net/tcp/tests.rs

Co-authored-by: Chris Denton <christophersdenton@gmail.com>

ChrisDenton

This looks great to me, thanks! Sorry testing end up being so involved.

ChrisDenton · 2023-06-20T10:57:30Z

@bors r+ rollup

bors · 2023-06-20T10:57:32Z

📌 Commit a0c757a has been approved by ChrisDenton

It is now in the queue for this repository.

eval-exec · 2023-06-20T10:59:12Z

Thank you.

…

On Tue, Jun 20, 2023 at 6:57 PM Chris Denton ***@***.***> wrote: ***@***.**** approved this pull request. This looks great to me, thanks! Sorry testing end up being so involved. — Reply to this email directly, view it on GitHub <#112464 (review)>, or unsubscribe </~https://github.com/notifications/unsubscribe-auth/ALCAINR7SJLSDNMQPA2SILLXMF6ZRANCNFSM6AAAAAAZAVVJSE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…llaumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#112464 (Fix windows `Socket::connect_timeout` overflow) - rust-lang#112720 (Rustdoc: search: color item type and reduce size to avoid clashing) - rust-lang#112762 (Sort the errors from arguments checking so that suggestions are handled properly) - rust-lang#112786 (change binders from tuple structs to named fields) - rust-lang#112794 (Fix linker failures when #[global_allocator] is used in a dependency) - rust-lang#112819 (Disable feature(unboxed_closures, fn_traits) in weird-exprs) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned thomcc Jun 9, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 9, 2023

This comment has been minimized.

Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch 2 times, most recently from bb125c5 to 80b5ea8 Compare June 9, 2023 14:18

ChrisDenton reviewed Jun 10, 2023

View reviewed changes

library/std/src/sys/windows/net.rs Outdated Show resolved Hide resolved

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch 2 times, most recently from 26720ce to 845c89c Compare June 10, 2023 14:04

This comment has been minimized.

Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from 1e6a863 to e23346b Compare June 11, 2023 01:06

This comment has been minimized.

Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from e23346b to 2953b71 Compare June 11, 2023 01:16

This comment has been minimized.

Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from 2953b71 to fc4cff2 Compare June 11, 2023 01:46

This comment has been minimized.

Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from fc4cff2 to 7bac07d Compare June 11, 2023 02:26

eval-exec requested a review from ChrisDenton June 11, 2023 03:16

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from 7bac07d to 11df849 Compare June 11, 2023 03:40

ChrisDenton reviewed Jun 12, 2023

View reviewed changes

rustbot assigned ChrisDenton and unassigned thomcc Jun 17, 2023

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from 11df849 to 7871e56 Compare June 17, 2023 08:16

This comment has been minimized.

Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from a4cce33 to ca83467 Compare June 17, 2023 16:20

This comment has been minimized.

Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from 13c995c to bf52e9b Compare June 18, 2023 07:42

This comment has been minimized.

Sign in to view

Add unit test to connect to an unreachable address

f65b5d0

Signed-off-by: Eval EXEC <execvy@gmail.com>

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from bf52e9b to f65b5d0 Compare June 18, 2023 08:00

eval-exec commented Jun 18, 2023

View reviewed changes

ChrisDenton reviewed Jun 20, 2023

View reviewed changes

library/std/src/net/tcp/tests.rs Outdated Show resolved Hide resolved

eval-exec and others added 2 commits June 20, 2023 18:43

Ignore connect_timeout unit test on SGX platform

30e1c1a

Co-authored-by: Chris Denton <christophersdenton@gmail.com>

Remove useless unit tests

a0c757a

eval-exec requested a review from ChrisDenton June 20, 2023 10:48

ChrisDenton approved these changes Jun 20, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2023

GuillaumeGomez mentioned this pull request Jun 20, 2023

Rollup of 6 pull requests #112839

Merged

bors merged commit 816b659 into rust-lang:master Jun 20, 2023

rustbot added this to the 1.72.0 milestone Jun 20, 2023

Fix windows Socket::connect_timeout overflow #112464

Fix windows Socket::connect_timeout overflow #112464

Conversation

eval-exec commented Jun 9, 2023 • edited Loading

rustbot commented Jun 9, 2023

This comment has been minimized.

eval-exec commented Jun 9, 2023

ChrisDenton commented Jun 10, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hartwork Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eval-exec Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomcc commented Jun 17, 2023

This comment has been minimized.

This comment has been minimized.

hartwork commented Jun 17, 2023

ChrisDenton commented Jun 17, 2023

hartwork commented Jun 17, 2023

hartwork commented Jun 17, 2023

eval-exec commented Jun 18, 2023

hartwork commented Jun 18, 2023 • edited Loading

eval-exec commented Jun 18, 2023 • edited Loading

connect to a reachable address:

connect to an unreachable address:

eval-exec commented Jun 18, 2023 • edited Loading

ChrisDenton commented Jun 18, 2023

This comment has been minimized.

Choose a reason for hiding this comment

ChrisDenton Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hartwork commented Jun 18, 2023

ChrisDenton left a comment

Choose a reason for hiding this comment

ChrisDenton commented Jun 20, 2023

bors commented Jun 20, 2023

eval-exec commented Jun 20, 2023 via email

Fix windows `Socket::connect_timeout` overflow #112464

Fix windows `Socket::connect_timeout` overflow #112464

eval-exec commented Jun 9, 2023 •

edited

Loading

hartwork Jun 17, 2023 •

edited

Loading

eval-exec Jun 17, 2023 •

edited

Loading

hartwork commented Jun 18, 2023 •

edited

Loading

eval-exec commented Jun 18, 2023 •

edited

Loading

eval-exec commented Jun 18, 2023 •

edited

Loading

ChrisDenton Jun 20, 2023 •

edited

Loading